Add MTS pitch and SysEx format conversion methods #7
Conversation
src/__tests__/conversion.spec.ts
Outdated
| @@ -38,6 +41,33 @@ describe('MIDI to frequency converter', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('Frequency to MTS converter', () => { | |||
| it('converts a known value', () => { | |||
There was a problem hiding this comment.
Should be it('converts known values', ...). This test case is doing multiple things.
There was a problem hiding this comment.
Thanks, I didn't connect the dots on that one.
There was a problem hiding this comment.
No problem. For the smoothest experience it would be nice if this it case was broken into multiple invididual test cases. Now if the first expect fails the rest won't run and you have to run the unit tests multiple times when things eventually break.
src/__tests__/conversion.spec.ts
Outdated
| }) | ||
|
|
||
| describe("Frequency to MTS sysex value", () => { | ||
| it('converts a known value', () => { |
There was a problem hiding this comment.
| it('converts a known value', () => { | |
| it('converts known values', () => { |
src/__tests__/conversion.spec.ts
Outdated
| }) | ||
|
|
||
| describe("MTS data hex string converter", () => { | ||
| it('converts a known value', () => { |
There was a problem hiding this comment.
| it('converts a known value', () => { | |
| it('converts known values', () => { |
src/conversion.ts
Outdated
| * @returns Frequency in Hertz. | ||
| */ | ||
| export function mtof(index: number) { | ||
| return 440 * Math.pow(2, (index - MIDI_NOTE_NUMBER_OF_A4) / 12); | ||
| } | ||
|
|
||
| function mtsRound(mtsValue: number): number { | ||
| return Math.round(mtsValue * 100000) / 100000; |
There was a problem hiding this comment.
How critical is it that this result is stable? Javascript's number type cannot represent decimals exactly under the hood.
There was a problem hiding this comment.
I think not very much. Currently there's no use case in SW for the mtsValue to be 128 or higher, but at the most, if for some reason someone used it in an MPE-style of tuning the upper bound would be 1920 or 2048. It only needs two-places of precision too.
I originally wanted to round to closest possible value in the three-byte representation which is why I made it a separate method. Decided it wasn't worth the debugging for now, but I kept the abstraction in.
|
Please do |
|
What's the status of this PR? I'm still getting a local diff when I do |
|
The new commits make the tests tighter, fix some rounding errors, and fixes the code style! :) |
src/conversion.ts
Outdated
| let lsb = mtsBytes[2]; | ||
|
|
||
| const noteNumber = mtsBytes[0] > 0x7f ? 0x7f : mtsBytes[0]; | ||
| if (noteNumber == 0x7f) { |
|
|
vsicurella
force-pushed
the
mts-methods
branch
from
83e63d5 to
18491d5
Compare
vsicurella
force-pushed
the
mts-methods
branch
from
18491d5 to
10532a4
Compare
| * @returns Uint8Array 3-byte of 7-bit MTS data | ||
| */ | ||
| export function mtsToMtsBytes(mtsValue: number): Uint8Array { | ||
| const noteNumber = Math.trunc(mtsValue); |
There was a problem hiding this comment.
Is there a test where mtsValue is negative? I'm wondering if we should worry about Math.trunc rounding towards zero instead of negative infinity. SW can produce silly scales with nanohertz frequencies.
There was a problem hiding this comment.
Fair call, I think that would only happen with bad usage but it would be good practice to clamp it to 0 otherwise you could be producing junk byte data.
|
Automated tests pass. Code seems fine. 👍 Feel free to merge if negative MTS values are nothing to worry about. |
MTS methods - "mtsToFrequency" is covered by "mtof" as the math is exactly the same!
I think I've actually set this PR up right this time lol